Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support InMemoryCache field policy drop functions, to allow cleanup when fields are removed from the cache #8078

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Apr 28, 2021

As I described in these comments, we recommend using the storage object for sharing private data between read and merge (and modify) functions, but there's currently no good way to make sure the storage object gets cleaned up when the field is removed from the cache.

This PR makes it possible to define a custom drop function for any field that needs special cleanup handling:

new InMemoryCache({
  typePolicies: {
    SomeType: {
      fields: {
        someField: {
          read(existing, { storage }) {
            if ("cached" in storage) return storage.cached;
            return storage.cached = computeExpensiveResult(...);
          },

          // Called just before this field is evicted/deleted/removed from the cache:
          drop(existing, { storage }) {
            delete storage.cached;
          },
        },
      },
    },
  },
})

The terminology of finalize comes from the existing OOP concept of a finalizer that runs before an object is garbage collected, though I decided to avoid the noun finalizer in favor of the present-tense transitive verb finalize, to match read and merge.

Edit: I decided to call this function drop instead of finalize, partly for brevity, partly inspired by Rust, but also because a finalizer (in the OOP sense) typically runs for a whole object, whereas these field policy drop functions run for specific fields.

@benjamn benjamn added this to the Release 3.4 milestone Apr 28, 2021
@benjamn benjamn self-assigned this Apr 28, 2021
@benjamn benjamn marked this pull request as draft April 28, 2021 23:47
@benjamn benjamn force-pushed the InMemoryCache-field-policy-finalize-function branch from 61d5bd4 to 8948dae Compare April 29, 2021 17:26
@benjamn benjamn changed the title Support InMemoryCache field policy finalize functions, to allow cleanup when fields are removed from the cache Support InMemoryCache field policy drop functions, to allow cleanup when fields are removed from the cache Apr 29, 2021
As I described in this comment, though I decided to call the new
function `drop` instead of `finalize`:
#8052 (comment)
@benjamn benjamn force-pushed the InMemoryCache-field-policy-finalize-function branch from de99635 to 640d504 Compare April 29, 2021 19:04
@benjamn benjamn requested a review from hwillson April 29, 2021 19:06
Comment on lines +1992 to +2001
cache.writeQuery({
query,
overwrite: true,
data: {
garbages: [
{ __typename: "Garbage", gid: 123, isToxic: false },
{ __typename: "Garbage", gid: 345, isToxic: true },
],
},
});
Copy link
Member Author

@benjamn benjamn Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see the overwrite: true option (implemented by 9410f17 in #7810) proving itself useful (for preventing the Cache data may be lost... warning) here.

});
});

itAsync("are called for fields within garbage collected objects", (resolve, reject) => {
Copy link
Member Author

@benjamn benjamn Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of the low/central level where I'm enforcing the calling of drop functions, any process that uses the EntityStore API to delete fields (or the objects that contain them) will trigger the appropriate drop functions. Even cache.gc() triggers drop functions. Pretty cool!

@benjamn benjamn marked this pull request as ready for review April 29, 2021 19:16
Comment on lines -756 to +768
context.store.getStorage(
isReference(objectOrReference)
? objectOrReference.__ref
: objectOrReference,
storeFieldName,
),
(context.store as EntityStore).group
.getStorage(objectOrReference, storeFieldName),
Copy link
Member Author

@benjamn benjamn Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These context.store.group.getStorage changes are not directly related to the goals of this PR, but are necessary to ensure the same storage object gets passed in the options for read, merge, and drop functions (assuming the underlying field is the same, of course). This is important so that (for example) drop can always clean up the same storage object previously populated by read or merge.

Implementation-wise, thanks to the assignPaths method above, unidentifiable/non-normalized objects are now able to use group.getStorage to look up the appropriate storage object using the string ID of the enclosing entity object, together with a path of properties leading to the object within the entity. By relying on permanent strings rather than temporary object references as lookup keys, this approach makes storage objects more stable for fields within non-normalized objects.

By contrast, for fields within normalized objects, the objectOrReference variable here will always be a Reference, which makes it easier to get the string ID directly from that Reference object. In other words, storage for fields within normalized objects was already "stable" in the sense used above, prior to this PR.

@benjamn
Copy link
Member Author

benjamn commented Apr 29, 2021

Something I haven't figured out yet: the best way to make sure drop functions are called after ephemeral read functions (that is, read functions for pure-@client fields whose existing cache data is nonexistent). Since nothing changes in the cache when those fields are "evicted," the current implementation of this PR probably won't detect their removal.

@benjamn benjamn marked this pull request as draft May 4, 2021 13:03
@hwillson hwillson removed this from the Release 3.4 milestone May 4, 2021
@hwillson hwillson added this to the Release 4.0 milestone May 4, 2021
@hwillson hwillson removed their request for review May 31, 2021 09:49
@hwillson hwillson removed this from the v4.0 - Planned milestone Jul 9, 2021
Base automatically changed from release-3.4 to main July 28, 2021 17:12
@jdmoliner
Copy link

Hi @benjamn,
Any news on this? i have been waiting for this feature for a long time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants